fix(enroll): eighth audit — atomic age-keygen, sops validation, step-12 depth#180
Merged
Conversation
…12 depth
- gen_password: validate output >= 16 chars; tr -d can produce a short
result on low-probability urandom runs; guard catches it before the
password is written to files
- age-keygen atomic write: replace `age-keygen -o AGE_KEY` (truncate then
write) with stdout capture to AGE_KEY.tmp then mv; interrupted write
previously left a partial key that re-runs treated as present and then
failed on age-keygen -y with a misleading "may be corrupt" message
- age-keygen stale .tmp cleanup: rm -f AGE_KEY.tmp at step 3 start,
consistent with HW_CONFIG.tmp, ENROLL_NIX.tmp, MINISIGN_CACHE_INFO.tmp
- sops output non-empty check: [[ -s _SECRETS_TMP ]] before mv; sops
exits 0 on some edge cases while producing empty output; empty ciphertext
would silently replace secrets.yaml and loop on re-run
- Remove redundant chmod 600 after mv: mv preserves permissions from
_SECRETS_TMP (created 600 via umask 077); the post-mv chmod was a no-op
that implied mv might change permissions
- $(seq 1 N) → {1..N} brace expansion in both polling loops: eliminates
subshell forks in tight 5s-interval loops (harmonia wait, syncd poll)
- sops-nix-activate failed-state check in step 12: oneshot services stay
in 'failed' not 'inactive' when they error; distinguishes decryption
failure from "still starting", with targeted re-enroll instructions
- Katello container liveness recheck in step 12: containers can be
OOM-killed during nix build (steps 10-11); sourceos-syncd shows active
but fails all Katello calls silently without this check
- Quote ${KATELLO_ADMIN_PW_FILE} in banner cat call: unquoted variable
is inconsistent with quoting elsewhere; safe in practice but fragile
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Findings and fixes
gen_passwordoutput not validated for minimum length —tr -dcan produce < 24 chars silently${#pw} -ge 16before returningage-keygen -o AGE_KEYis non-atomic — interrupted write leaves partial key; re-run sees it as present and failsage-keygen -ywith no self-healing> AGE_KEY.tmp+mv; adds stale.tmpcleanup at step startsops --encryptoutput not validated — exits 0 on some edge cases with empty output; empty ciphertext silently replacessecrets.yaml[[ -s _SECRETS_TMP ]]beforemvchmod 600aftermvimpliesmvmight change permissionsmvpreserves source permissions$(seq 1 12)and$(seq 1 6)fork subprocesses in tight polling loops{1..12}/{1..6}brace expansionsops-nix-activatefailure not detected in step 12 — oneshot staysfailednotinactive;sourceos-syncdshows active but can never get its credentialsystemctl is-failed --quiet sops-nix-activatecheck with targeted diagnosticsourceos-syncdactive but all Katello calls fail silentlydocker ps --filter name=katello | wc -lat end of step 12${KATELLO_ADMIN_PW_FILE}inbanner()catcallTest plan
bash -n scripts/enroll.sh— no syntax errorsage-keygenmid-write → re-run regenerates cleanly (no "key file may be corrupt" with partial file)sops --encryptto produce empty output → guard fires before mv